Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New Redis integration #3018 #3100 #4180

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

New Redis integration #3018 #3100 #4180

wants to merge 2 commits into from
+483 −9

Conversation

washerath
Copy link

@washerath washerath commented Dec 8, 2023 •
edited
Loading

Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

@washerath
Initial commit
9f1ad62 
Signed-off-by: Wasantha Herath <[email protected]>
@washerath
ExpirationKCVSRedisCache compatibility issue fix
7a7a21c 
Signed-off-by: Wasantha Herath <[email protected]>
@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Dec 11, 2023
@FlorianHockmann FlorianHockmann mentioned this pull request Jan 17, 2024
Open
FlorianHockmann
FlorianHockmann reviewed Jan 17, 2024
View reviewed changes
Copy link
Member

@FlorianHockmann FlorianHockmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for contributing this, @washerath! Before diving too deep into reviewing the changes, I have some high-level questions:

  1. Would it make sense to move this into an independent module like janusgraph-redis? That way, we wouldn't have to let janusgraph-core depend on a Redis library and I think it would also make it easier to maybe add other cache implementations in the future.
  2. Are you already using this implementation yourself? Maybe even in production? Could you share some experiences with it? (This is of course not necessary. I'm just curios here.)
  3. @li-boxuan asked in your other PR about performance. Do you have anything to share about that?

I also think that we need some documentation for this. Otherwise users won't even know that it exists. Maybe we should even add a new section "Cache Backends" similar to our "Storage Backends" and "Index Backends".
But we can also do that in a follow-up PR I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FlorianHockmann FlorianHockmann FlorianHockmann left review comments

Assignees
No one assigned
Labels
cla: external Externally-managed CLA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@washerath @FlorianHockmann @janusgraph-bot